Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

tests/private-access-routes.test.ts (1)

441-441: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Widened error assertions (toEqual → toMatchObject) lose exact-shape verification for error envelopes.

This PR's stated goal is sanitized, structured API errors (Line: error logging and HTTP responses use sanitized details and stable codes per PR summary). Switching these ~25 assertions from strict toEqual to toMatchObject is likely needed because the envelope now includes an additional field (e.g. a stable code), but it also means these tests would silently pass if an unexpected/sensitive extra field leaked into the response alongside error. Given the explicit privacy/sanitization focus of this PR, consider asserting the full expected envelope (e.g. { error: "...", code: "..." }) instead of a partial match, so a future regression that adds a leaked field is caught.

Also applies to: 464-464, 657-657, 671-671, 709-709, 751-751, 860-860, 879-879, 936-936, 1289-1289, 1899-1899, 1963-1963, 2208-2208, 2381-2381, 2445-2445, 2463-2463, 2561-2561, 2684-2684, 2713-2713, 2744-2744, 2927-2927, 2958-2958, 3240-3240, 3340-3340, 3893-3893

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/private-access-routes.test.ts` at line 441, Widened error assertions no
longer verify the complete sanitized response envelope. Update the listed
assertions in tests/private-access-routes.test.ts, including the
payload(response) checks, to use strict equality with the expected error message
and stable code fields, matching the structured API error shape and preventing
unexpected fields from being accepted.

tests/client-secret-surface.test.ts (1)

53-85: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Import-chain scan misses dynamic import() calls.

localImports only inspects top-level ImportDeclaration/ExportDeclaration statements. A client module that reaches env.ts via await import("@/lib/env") (or any dynamic import inside a function) would silently bypass serverEnvImportChains(), defeating the purpose of this guard test.

♻️ Suggested addition to also catch dynamic imports

 function localImports(file: string) {
   const cached = importCache.get(file);
   if (cached) return cached;
   const text = readFileSync(file, "utf8");
   const source = ts.createSourceFile(file, text, ts.ScriptTarget.Latest, true);
   const specifiers: string[] = [];
-  for (const statement of source.statements) {
-    if (ts.isImportDeclaration(statement) && ts.isStringLiteral(statement.moduleSpecifier)) {
-      ...
-    }
-  }
+  const visit = (node: ts.Node) => {
+    if (
+      ts.isCallExpression(node) &&
+      node.expression.kind === ts.SyntaxKind.ImportKeyword &&
+      node.arguments[0] &&
+      ts.isStringLiteral(node.arguments[0])
+    ) {
+      specifiers.push(node.arguments[0].text);
+    }
+    ts.forEachChild(node, visit);
+  };
+  for (const statement of source.statements) {
+    if (ts.isImportDeclaration(statement) && ts.isStringLiteral(statement.moduleSpecifier)) {
+      ...
+    }
+  }
+  visit(source);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/client-secret-surface.test.ts` around lines 53 - 85, The localImports
function only detects static imports and exports, so dynamic import()
dependencies are omitted from the chain scan. Extend its TypeScript AST
traversal to find CallExpressions whose expression is the import keyword,
collect string-literal module specifiers, and resolve them through
resolveLocalImport alongside existing specifiers; ensure nested dynamic imports
are detected, not just top-level statements.

tests/differentials-route.test.ts (1)

93-114: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy lift

Console.error assertions verify the mock's own hardcoded log call, not the real registry-corpus logging path.

bestEffortSyncDifferentialRows is fully mocked here and reimplements the console.error("[differentials] registry corpus sync failed", ...) call itself (Line 106-109), duplicating the shape of the real implementation in src/lib/registry-corpus.ts (console.error(\[${scope}] registry corpus sync failed`, safeErrorLogDetails(error))). The assertions at Lines 249-252 and 282-285 therefore just confirm the mock does what it's told, not that the production sanitized-logging path (safeErrorLogDetails) actually produces this shape. If registry-corpus.ts`'s real scope string or sanitization changes, this test won't catch the regression.

Consider exercising the real bestEffortSyncDifferentialRows/shared best-effort helper (mocking only the embedding call, as done in tests/registry-corpus.test.ts) rather than mocking the whole module's error-handling behavior.

Also applies to: 228-286

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/differentials-route.test.ts` around lines 93 - 114, Replace the fully
mocked bestEffortSyncDifferentialRows implementation in mockRuntime with the
real registry-corpus error-handling path, mocking only the embedding dependency
as in registry-corpus.test.ts. Remove the mock’s hardcoded console.error and
return values, then configure the embedding call to fail so assertions at the
affected test cases verify the production safeErrorLogDetails-based sanitized
logging and real scope string.

src/app/api/medications/[slug]/route.ts (1)

114-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider extracting the seed-retry pattern into a shared helper.

The catch/log/refetch/throw-only-if-still-missing logic here duplicates the pattern already extracted as fetchOwnerDifferentialRowsWithSeed for the differentials route. Since medication-seed.ts doesn't expose an equivalent wrapper, this shell is hand-rolled inline — and, per the PR objective of unifying seed-failure handling across routes, is likely duplicated again in the sibling routes (differentials/[slug], differentials/presentations/[slug], registry/records/[slug]) not included in this batch.

♻️ Suggested extraction into medication-seed.ts

+export async function fetchOwnerMedicationRowWithSeed(
+  supabase: AdminClient,
+  ownerId: string,
+  slug: string,
+  fetchRecord: () => Promise<MedicationRecordRow | null>,
+): Promise<MedicationRecordRow | null> {
+  let row = await fetchRecord();
+  if (!row) {
+    const { count, error: countError } = await supabase
+      .from("medication_records")
+      .select("id", { count: "exact", head: true })
+      .eq("owner_id", ownerId);
+    if (countError) throw new Error(countError.message);
+    if ((count ?? 0) === 0) {
+      let seedError: unknown = null;
+      try {
+        await ensureMedicationsSeeded(supabase, ownerId);
+      } catch (error) {
+        seedError = error;
+        console.error("[medications] auto-seed failed", safeErrorLogDetails(error));
+      }
+      row = await fetchRecord();
+      if (!row && seedError) throw seedError;
+    }
+  }
+  return row;
+}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/api/medications/`[slug]/route.ts around lines 114 - 122, Extract the
medication seed retry behavior into a shared helper in medication-seed.ts,
matching fetchOwnerDifferentialRowsWithSeed: perform the initial fetch, attempt
ensureMedicationsSeeded, log failures with safeErrorLogDetails, refetch
afterward, and rethrow the seed error only when the record remains missing.
Update the medications route and sibling record routes to use this helper
instead of duplicating the inline seedError/catch/refetch logic.

✅ Autofix completed

tests/ui-smoke.spec.ts (1)

1930-1940: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Redundant fixed-delay re-assertion.

toContainText already polls/retries internally, so the second identical assertion after waitForTimeout(600) mostly just re-verifies via a hardcoded sleep rather than an event-driven wait. If the intent is to confirm the status doesn't later flip (e.g., due to a delayed async fetch), consider documenting that intent in a comment, or use expect.poll with a timeout tied to the specific async event instead of an arbitrary sleep, to reduce flakiness risk.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/ui-smoke.spec.ts` around lines 1930 - 1940, Replace the fixed
waitForTimeout and redundant second toContainText assertion in the differential
source-status test with an event-driven expect.poll or another wait tied to the
delayed async request. If retaining the second verification to ensure the status
does not change, document that intent and use a bounded, meaningful timeout
rather than a hardcoded sleep.

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".